fix(tests): HLL fuzz test — deterministic AddBytes (Otto-281)#482
Merged
fix(tests): HLL fuzz test — deterministic AddBytes (Otto-281)#482
Conversation
The `fuzz: HLL estimate within theoretical error bound` test in `tests/Tests.FSharp/Properties/Fuzz.Tests.fs` was calling `hll.Add i` on integer keys, which routes through `HashCode.Combine` — process-randomized by .NET design (anti- hash-flooding) and acknowledged in the `HyperLogLog.Add` docstring as a "process-randomization caveat (Otto-281 audit)". Different CI processes produced different bucket-landings for the same int sequence, occasionally pushing the estimate past the 4% tolerance and flaking the test. Most recent flake: PR #480 ubuntu-24.04 run 24932270073 (a memory-only PR that made no code changes — pure pre-existing test instability). Per Otto-281 (DST-exempt is deferred bug — fix the determinism not the comment), this routes int keys through `AddBytes` with a canonical 4-byte `BitConverter.GetBytes` representation. The hash distribution properties HLL needs (uniform avalanche over 64 bits) are preserved by `XxHash3.HashToUInt64`, and the result is deterministic across runs. Same fix shape as PR #478 (SharderInfoTheoreticTests). Rationale comment inline so future readers understand why we're not just calling `.Add` directly (Otto-282 — write the why). Verified: 3/3 fuzz tests pass locally.
There was a problem hiding this comment.
Pull request overview
Stabilizes the FsCheck fuzz property for HyperLogLog accuracy by removing process-randomized hashing from the test input path, preventing CI flakes due to run-to-run bucket changes.
Changes:
- Add an Otto-281 rationale comment explaining the prior flake mechanism and the deterministic hashing approach.
- Update the HLL fuzz test to feed integer keys via
AddBytesrather thanAdd, avoidingHashCode.Combineprocess randomization.
This was referenced Apr 25, 2026
AceHack
added a commit
that referenced
this pull request
Apr 25, 2026
Aaron 2026-04-25 standing meta-rule above Otto-281: > "we never want to use random seed pins to cheat by not > fully testing if you understand what I mean." > "the general rule is dont use DST and determinism to > avoid edge cases handling." > "tests are all deterministic but the real world isn't, > our tests are trying to test all the edge cases of the > real world but in a deterministic way not reduce scope > by eliminating edge cases of the real world in our tests > with determinism. that will lead to more robust tests." The principle: determinism is the WAY to test chaos reproducibly (so bugs replay), NOT the REASON to skip chaos. The real world is non-deterministic — tests should deterministically exercise every flavor of chaos the algorithm will encounter in production. The discriminator question for any "make it deterministic" fix: does it INVOKE the algorithm's actual contract (legitimate — algorithm has documented input invariants you're satisfying) or SHRINK the test's coverage of what the algorithm is supposed to handle (cheat)? Same input space via deterministic primitive = fine (invoke contract). Narrower input space because broader revealed problems = cheat. Empirical verification this session: PR #482 HLL fuzz fix is LEGITIMATE per discriminator. HLL theorem requires uniform 64-bit hashes; HashCode.Combine violated contract; XxHash3 satisfies contract; same n values FsCheck generates are still tested. 500-trial × 5-offset sweep shows max error 1.96% vs 4% bound — contract-satisfied means bound holds. Composes with Otto-281 (fix the determinism — Otto-285 is the meta-rule above), Otto-272 (DST as substrate), Otto-264 (rule of balance — paired coverage verification), Otto-282 (comment WHY discriminator), Otto-248 (never ignore flakes — flakes ARE the contract-violation signal). CLAUDE.md candidate; deferred to maintainer discretion per Otto-283. Memory-reference lint verified: 497/497 refs resolve.
AceHack
added a commit
that referenced
this pull request
Apr 25, 2026
…484) GitHub's release-asset CDN occasionally returns 502 / 5xx errors. Most recent observed: 2026-04-25 ~13:52 UTC, hit PR #481 (CodeQL csharp install) + PR #482 (markdownlint install via mise:elan). Both PRs were unrelated to the network — pure environmental flake. Per Otto-285 (don't use determinism to avoid edge-case handling — when we can't control real-world chaos, react to it algorithmically), the install script's curl call now includes: - `--retry 5` attempts - `--retry-delay 2` (initial; curl exponential-backs to 2/4/8/16/32 s) - `--retry-all-errors` (so 4xx/5xx errors retry too — curl's default only retries connect / dns / 408 / 429 / 5xx-with-Retry-After header) `-fsSL` semantics preserved — if all 5 attempts hit the same transient, the final exit code still fails, so we don't silently swallow persistent CDN outages. Aaron 2026-04-25 affirmation: "that is the right answer we cant control that part of the real world environment we have to react to it, good call." Verified: `bash -n` syntax check passes. Composes with Otto-285 (DST is not edge-case avoidance) + Otto-281 (fix determinism not the comment) + Otto-264 (rule of balance — every CI flake we hit gets a counterweight fix).
Resolves two copilot review threads on PR #482: 1. **Comment / threshold mismatch.** Earlier comment said "we allow 3%" but the assertion is `err < 0.04` (4%). Comment now says "we allow 4%" + cites the empirical 1.96% max observed across the 500-trial sweep that informed the bound. 2. **Endianness + per-element allocation.** `BitConverter.GetBytes` is endianness-dependent on the host (big-endian hosts produce different bytes for the same int) and allocates a new `byte[]` per call. Replaced with `BinaryPrimitives.WriteInt32LittleEndian` writing into a single `byte[4]` allocated once outside the loop. Same determinism property + zero per-element alloc + cross- platform stable. Net: same Otto-281 fix shape, now also satisfying Otto-282 (write the WHY visibly for the endianness pin and the allocation discipline). Verified: build green, HLL fuzz test passes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
fuzz: HLL estimate within theoretical error boundtest was callinghll.Add ion integer keys, routing throughHashCode.Combine(process-randomized by .NET). Different CI runs produced different bucket-landings, occasionally pushing the estimate past the 4% tolerance and flaking the test.Most recent flake: PR #480 ubuntu-24.04 (a memory-only PR that made zero code changes — purely pre-existing test instability surfacing on a clean run).
Per Otto-281 (DST-exempt is deferred bug — fix the determinism, not the comment), routes int keys through
AddByteswithBitConverter.GetBytesfor canonical byte representation. The hash distribution HLL needs (uniform avalanche over 64 bits) is preserved byXxHash3.HashToUInt64, and the result is deterministic across runs.Same fix shape as PR #478 (SharderInfoTheoreticTests Otto-281 fix). One-line rationale comment inline per Otto-282 (write the why so future readers don't ask).
Test plan
dotnet build -c Release— 0 warnings, 0 errors🤖 Generated with Claude Code